-
-
Notifications
You must be signed in to change notification settings - Fork 3
feat: pass settings via interface to the client #301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Warning Rate limit exceeded@simPod has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (11)
WalkthroughThe codebase was refactored to replace all usages of raw associative arrays for passing settings to client and request interfaces with a new abstraction: Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test/Caller
participant Provider as ArraySettingsProvider/EmptySettingsProvider
participant Client as PsrClickHouseClient/PsrClickHouseAsyncClient
participant ReqSet as RequestSettings
Test->>Provider: new ArraySettingsProvider(settings)
Test->>Client: call method(..., Provider)
Client->>ReqSet: new RequestSettings(defaultProvider, Provider)
ReqSet->>defaultProvider: get()
ReqSet->>Provider: get()
ReqSet-->>Client: merged settings
Client-->>Test: result/output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–25 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #301 +/- ##
==========================================
+ Coverage 94.66% 94.70% +0.04%
==========================================
Files 40 42 +2
Lines 750 756 +6
==========================================
+ Hits 710 716 +6
Misses 40 40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🔭 Outside diff range comments (4)
src/Client/PsrClickHouseClient.php (4)
57-64: Invalid default object in parameter; switch to nullable + fallbackReplace the default with nullable and fallback at call site.
- public function executeQuery(string $query, SettingsProvider $settings = new EmptySettingsProvider()): void + public function executeQuery(string $query, ?SettingsProvider $settings = null): void { try { - $this->executeRequest($query, params: [], settings: $settings); + $this->executeRequest($query, params: [], settings: $settings ?? new EmptySettingsProvider()); } catch (UnsupportedParamType) { absurd(); } }
90-109: Invalid default object in parameter; switch to nullable + fallback- public function selectWithParams( - string $query, - array $params, - Format $outputFormat, - SettingsProvider $settings = new EmptySettingsProvider(), - ): Output { + public function selectWithParams( + string $query, + array $params, + Format $outputFormat, + ?SettingsProvider $settings = null, + ): Output { @@ $response = $this->executeRequest( @@ - settings: $settings, + settings: $settings ?? new EmptySettingsProvider(), );
206-211: Invalid default object in parameter; switch to nullable + fallback- public function insertWithFormat( + public function insertWithFormat( Table|string $table, Format $inputFormat, string $data, - SettingsProvider $settings = new EmptySettingsProvider(), + ?SettingsProvider $settings = null, ): void { @@ - $this->executeRequest( + $this->executeRequest( @@ - settings: $settings, + settings: $settings ?? new EmptySettingsProvider(), );Also applies to: 221-227
233-239: Invalid default object in parameter; switch to nullable + fallback (and pass non-null to RequestSettings)- public function insertPayload( + public function insertPayload( Table|string $table, Format $inputFormat, StreamInterface $payload, array $columns = [], - SettingsProvider $settings = new EmptySettingsProvider(), + ?SettingsProvider $settings = null, ): void { @@ - $request = $this->requestFactory->initRequest( + $request = $this->requestFactory->initRequest( new RequestSettings( $this->defaultSettings, - $settings, + $settings ?? new EmptySettingsProvider(), ), ['query' => $sql], );Also applies to: 258-263
♻️ Duplicate comments (1)
src/Client/PsrClickHouseAsyncClient.php (1)
35-36: Same note about “new” in parameter defaults as in the interface.Constructor default new EmptySettingsProvider() also depends on PHP ≥ 8.1 support.
🧹 Nitpick comments (7)
tests/Client/SelectTest.php (1)
172-172: LGTM: settings passed via provider.Call now uses ArraySettingsProvider as intended. Optionally use a named argument for clarity: settings: new ArraySettingsProvider([...]).
src/Settings/EmptySettingsProvider.php (1)
9-12: Add PHPStan return annotation for consistency.Mirror the interface’s phpstan alias on the concrete implementation to help static analysis.
- public function get(): array + /** @phpstan-return Settings */ + public function get(): arraytests/Client/Http/RequestSettingsTest.php (1)
18-20: LGTM: Merge semantics covered.Test validates override (database) and retention (a/b). Consider adding a case with overlapping non-database keys to assert second provider precedence and, if booleans are allowed, a boolean setting to ensure proper handling.
src/Settings/ArraySettingsProvider.php (1)
15-18: Nit: add phpstan return type for stronger static typing.Add an explicit phpstan return annotation to mirror the constructor param type alias.
- public function get(): array + /** @phpstan-return Settings */ + public function get(): arraysrc/Client/Http/RequestSettings.php (1)
12-13: Consider making settings readonly.This is config assembled in ctor; mark as readonly to prevent accidental mutation.
- public array $settings; + public readonly array $settings;src/Client/PsrClickHouseAsyncClient.php (1)
26-26: Nit: remove unused phpstan import-type.The imported Settings type alias isn’t referenced in this class.
-/** @phpstan-import-type Settings from SettingsProvider */src/Client/ClickHouseClient.php (1)
19-19: Nit: remove unused phpstan import-type.The Settings type alias isn’t referenced in this interface.
-/** @phpstan-import-type Settings from SettingsProvider */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Client/ClickHouseAsyncClient.php(1 hunks)src/Client/ClickHouseClient.php(5 hunks)src/Client/Http/RequestSettings.php(1 hunks)src/Client/PsrClickHouseAsyncClient.php(4 hunks)src/Client/PsrClickHouseClient.php(7 hunks)src/Settings/ArraySettingsProvider.php(1 hunks)src/Settings/EmptySettingsProvider.php(1 hunks)src/Settings/SettingsProvider.php(1 hunks)tests/Client/Http/RequestFactoryTest.php(3 hunks)tests/Client/Http/RequestSettingsTest.php(2 hunks)tests/Client/SelectTest.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
tests/Client/SelectTest.php (5)
src/Settings/ArraySettingsProvider.php (1)
ArraySettingsProvider(8-19)src/Client/ClickHouseClient.php (1)
select(52-56)src/Client/PsrClickHouseClient.php (1)
select(78-88)src/Format/JsonCompact.php (1)
JsonCompact(14-29)src/Output/JsonCompact.php (1)
JsonCompact(18-52)
tests/Client/Http/RequestFactoryTest.php (2)
src/Settings/ArraySettingsProvider.php (1)
ArraySettingsProvider(8-19)src/Settings/EmptySettingsProvider.php (1)
EmptySettingsProvider(7-13)
src/Settings/EmptySettingsProvider.php (2)
src/Settings/SettingsProvider.php (1)
get(11-11)src/Settings/ArraySettingsProvider.php (1)
get(15-18)
tests/Client/Http/RequestSettingsTest.php (1)
src/Settings/ArraySettingsProvider.php (1)
ArraySettingsProvider(8-19)
src/Settings/ArraySettingsProvider.php (5)
src/Client/Http/RequestSettings.php (1)
__construct(15-20)src/Client/PsrClickHouseClient.php (1)
__construct(47-55)src/Client/PsrClickHouseAsyncClient.php (1)
__construct(31-38)src/Settings/EmptySettingsProvider.php (1)
get(9-12)src/Settings/SettingsProvider.php (1)
get(11-11)
src/Client/ClickHouseAsyncClient.php (4)
src/Settings/EmptySettingsProvider.php (1)
EmptySettingsProvider(7-13)src/Client/ClickHouseClient.php (2)
select(52-56)selectWithParams(71-76)src/Client/PsrClickHouseClient.php (2)
select(78-88)selectWithParams(90-110)src/Client/PsrClickHouseAsyncClient.php (2)
select(45-51)selectWithParams(58-79)
src/Settings/SettingsProvider.php (2)
src/Settings/EmptySettingsProvider.php (1)
get(9-12)src/Settings/ArraySettingsProvider.php (1)
get(15-18)
🪛 GitHub Check: Infection
src/Client/PsrClickHouseClient.php
[warning] 71-71:
Escaped Mutant for Mutator "MethodCallRemoval":
@@ @@
}
public function executeQueryWithParams(string $query, array $params, SettingsProvider $settings = new EmptySettingsProvider()): void
{
-
$this->executeRequest($this->sqlFactory->createWithParameters($query, $params), params: $params, settings: $settings);
- }
public function select(string $query, Format $outputFormat, SettingsProvider $settings = new EmptySettingsProvider()): Output
{
🔇 Additional comments (18)
tests/Client/SelectTest.php (1)
17-17: Import update looks correct.Importing ArraySettingsProvider aligns the test with the refactored API.
tests/Client/Http/RequestFactoryTest.php (3)
16-17: Imports aligned with new settings abstraction.ArraySettingsProvider and EmptySettingsProvider imports are correct.
40-42: LGTM: RequestSettings constructed from providers.The test asserts the resulting query string; ensure RequestSettings preserves deterministic merge order so the params appear as expected.
If helpful, add a quick assertion that the internal merged settings array order is stable:
self::assertSame(['database', 'max_block_size'], array_keys($requestSettings->settings));
88-90: LGTM: Empty settings via explicit provider.Using EmptySettingsProvider removes ambiguity vs empty arrays and matches the API contract.
src/Settings/EmptySettingsProvider.php (1)
7-13: PHP minimum version requirement satisfied
- composer.json declares
"php": "^8.3", which exceeds the PHP 8.2+ requirement forreadonlyclasses.- No occurrences of
newin default parameter values were found in the codebase.No changes are needed.
tests/Client/Http/RequestSettingsTest.php (1)
9-9: Import is correct.ArraySettingsProvider import matches the constructor changes.
src/Settings/ArraySettingsProvider.php (1)
8-18: Solid, minimal provider implementation (immutable, typed).The class is clean, immutable, and aligns with the new SettingsProvider abstraction.
src/Client/Http/RequestSettings.php (1)
15-20: Merge precedence is correct (query overrides default).Using $querySettings->get() + $defaultSettings->get() ensures query keys take precedence over defaults.
src/Client/ClickHouseAsyncClient.php (2)
20-24: Consistent interface refactor to SettingsProvider.Signatures and generics docs remain coherent; aligns with the new abstraction across the codebase.
Also applies to: 32-37
20-24: No action needed: PHP ≥ 8.1 support is guaranteed by composer.json
Composer.json requires PHP ^8.3, which fully covers the “new in parameter defaults” feature added in PHP 8.1. Usingnew EmptySettingsProvider()as a default parameter is safe—no changes required.src/Client/PsrClickHouseAsyncClient.php (3)
27-27: LGTM: making the client readonly is a good fit.The class being readonly enforces immutability of promoted properties and matches its role.
45-49: Method signatures align with interface and propagate SettingsProvider correctly.Defaults and typing are consistent; select delegates to selectWithParams cleanly.
Also applies to: 62-63
90-92: Good: executeRequest now takes SettingsProvider directly.Keeps request preparation consistent with RequestSettings merging of default + query settings.
src/Client/ClickHouseClient.php (2)
26-26: Refactor looks consistent and improves API clarity.Moving to SettingsProvider across execute/select/insert methods standardizes configuration passing.
Also applies to: 52-56, 71-76, 88-94, 103-108, 118-124
26-26: PHP ≥8.0 support fornew Class()defaults is satisfied
Your composer.json specifies^8.3, so usingnew EmptySettingsProvider()as a default parameter is fully supported. All imports ofSettingsProviderandEmptySettingsProviderare in use. No further changes required.src/Client/PsrClickHouseClient.php (3)
24-26: New SettingsProvider imports look goodImporting EmptySettingsProvider and SettingsProvider aligns with the new abstraction.
282-296: Usage of SettingsProvider in executeRequest looks correctCombining default and per-call settings via RequestSettings and passing params through RequestOptions is coherent with the new abstraction.
41-41: Platform requirement verified – no changes neededThe project’s composer.json declares
"php": "^8.3", which already satisfies the PHP 8.2+ requirement forreadonly class. No adjustment is necessary.
61027e2 to
50ce893
Compare
2aae305 to
8eb584f
Compare
371c751 to
35ccb68
Compare
Summary by CodeRabbit